Fix/1571 Incremental: Optionally load or ignore records with cursor_path missing or None value #1576

willi-mueller · 2024-07-10T11:43:49Z

Description

This PR:
Allows users to specify what happens when the value at the incremental cursor path is None or the field is missing in a row. It also unifies the handling of null values in pandas/arrow with python objects.

Consider the following example data where created_at is the incremental cursor path:

data_1 = [
  {"a": 1, "created_at": 1},
  {"a": 2, "created_at": None},
]

data_2 = [
  {"a": 1, "created_at": 1},
  {"a": 2},
]

The options are:

incremental(..., on_cursor_value_missing="raise"). This will raise IncrementalCursorPathHasValueNone for data_1 and IncrementalCursorPathMissing for data_2.
incremental(..., on_cursor_value_missing="include"). This will load all rows for both data_1 and data_2 respectively.
incremental(..., on_cursor_value_missing="exclude"). This will load only the first row for both data_1 and data_2 respectively.

This PR also adds documentation on how to load data with None at the cursor path incrementally

All outlined features are implemented and tested for all 4 data formats (object, pandas, arrow-table, arrow batch). However, JSON path cursors are still only supported for JSON objects but not for arrow and pandas.

Done in collaboration with @francescomucio

TODO

docs: explain new parameter
docs: explain how we can leverage add_map() to add default values

Related Issues

Resolves Pipeline crashes when single record has None value at incremental cursor path #1571

netlify · 2024-07-10T11:44:06Z

✅ Deploy Preview for dlt-hub-docs ready!

Name	Link
🔨 Latest commit	`4ce384a`
🔍 Latest deploy log	https://app.netlify.com/sites/dlt-hub-docs/deploys/66d1b5e64a6fec0008236e69
😎 Deploy Preview	https://deploy-preview-1576--dlt-hub-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

VioletM · 2024-07-29T09:23:21Z

@willi-mueller this is an important feature, thank you!
I have a question: records with None value at the incremental cursor will be re-loaded each time, right? Could maybe some fall-back cursor be specified in this case?

rudolfix

@willi-mueller incremental is a minefield ;> please read my comments. we can simplify code in json transform but there are edge cases in arrow. please fix what you can and we'll work on the tests

dlt/extract/incremental/__init__.py

dlt/extract/incremental/transform.py

dlt/extract/incremental/typing.py

tests/extract/test_incremental.py

dlt/extract/incremental/transform.py

willi-mueller · 2024-07-30T07:34:01Z

@willi-mueller this is an important feature, thank you! I have a question: records with None value at the incremental cursor will be re-loaded each time, right? Could maybe some fall-back cursor be specified in this case?

Thank you @VioletM. You raise an important question! Yes, each time the None values appears in the source it will be processed. My proposal would be to use add_map() to specify the default fallback cursor.

What do you think about the proposed changes in docs/website/docs/general-usage/incremental-loading.md proposed here: #1650?

How can we mention your point there too?

adds more tests

…ow. Fails for pandas & pyarrow implements handling of cursor_path value is None for nested JSON paths

…alue and is_above_end_value

…al function is called

…ow_initial_value and is_above_end_value This reverts commit 733d1c0.

(cherry picked from commit 95db6c7 and 6d682da) incorporates docs changes from review

…trics

… Fails for arrow-batch

willi-mueller · 2024-08-08T16:42:47Z

@rudolfix Currently, the following test is failing:
tests/pipeline/test_pipeline_extra.py::test_dump_trace_freeze_exception

The reason is that before this PR, the following exception was ignored: AttributeError: 'TestRow' object has no attribute 'get' when trying to access TestRow.get("id"). Instead, the IncrementalCursorPathMissing was raised.

However, in this PR, I wanted to:

we do not ignore the exception when the record object does not implement __get__().
assume that every row implements keys().

Do you think these two points are appropriate?

Just to let the test suite pass, I restored the old behavior in 06aff12 passing any exception and using a run-time type check to decide whether to use keys() or hasattr().

Further, the test case is concerned with how dlt handles traces and uses the incremental only to produce a certain trace and it is not testing the incremental implementation.

What would you advise?

…::test_dump_trace_freeze_exception

rudolfix

I kind of like it now! Please see the comments. thanks for all tests and docs!

dlt/extract/incremental/transform.py

tests/extract/test_incremental.py

…l-cursor-path

rudolfix

LGTM!

… with cursor_path missing or None value (#1576) * allows specification of what happens on cursor_path missing or cursor_path having the value None: raise differentiated exceptions, exclude row, or include row. * Documents handling None values at the incremental cursor * fixes incremental extract crashing if one record has cursor_path = None * test that add_map can be used to transform items before the incremental function is called * Unifies treating of None values for python Objects (including pydantic), pandas, and arrow --------- Co-authored-by: Marcin Rudolf <[email protected]>

willi-mueller force-pushed the fix/1571-tolerate-record-without-incremental-cursor-path branch from 09001ab to 858ec47 Compare July 12, 2024 12:57

willi-mueller changed the title ~~Fix/1571 tolerate record without incremental cursor path~~ Fix/1571 tolerate record with None value at incremental cursor path Jul 12, 2024

willi-mueller force-pushed the fix/1571-tolerate-record-without-incremental-cursor-path branch from dff327d to 2e67119 Compare July 22, 2024 13:52

VioletM added the community This issue came from slack community workspace label Jul 25, 2024

willi-mueller force-pushed the fix/1571-tolerate-record-without-incremental-cursor-path branch from 2e67119 to 2588d7f Compare July 25, 2024 08:59

willi-mueller marked this pull request as ready for review July 25, 2024 09:00

willi-mueller force-pushed the fix/1571-tolerate-record-without-incremental-cursor-path branch from 2588d7f to 95db6c7 Compare July 25, 2024 09:09

willi-mueller mentioned this pull request Jul 25, 2024

Pipeline crashes when single record has None value at incremental cursor path #1571

Closed

3 tasks

willi-mueller requested review from rudolfix and sh-rp July 25, 2024 09:12

willi-mueller changed the title ~~Fix/1571 tolerate record with None value at incremental cursor path~~ Fix/1571 Optionally load records with None value at incremental cursor path Jul 25, 2024

rudolfix requested changes Jul 29, 2024

View reviewed changes

willi-mueller changed the title ~~Fix/1571 Optionally load records with None value at incremental cursor path~~ Fix/1571 Incremental: Optionally load records with None value at cursor_path Jul 31, 2024

willi-mueller mentioned this pull request Jul 31, 2024

Docs/1571 docs on handling NULL values at incremental cursor path #1650

Merged

willi-mueller added 13 commits August 2, 2024 17:13

adds failing test

bbfbb04

incremental extract does not crash if one record has cursor_path = None

b2efc6e

adds more tests

implements ability to raise on cursor_path = None or to include the r…

99327f9

…ow. Fails for pandas & pyarrow implements handling of cursor_path value is None for nested JSON paths

renames start_out_of_range and end_out_of_range to is_below_initial_v…

bfda7de

…alue and is_above_end_value

test that add_map can be used to transform items before the increment…

c1e765f

…al function is called

Reverts renaming of start_out_of_range and end_out_of_range to is_bel…

147772e

…ow_initial_value and is_above_end_value This reverts commit 733d1c0.

Documents handling NULL values at incremental cursor

36957fa

(cherry picked from commit 95db6c7 and 6d682da) incorporates docs changes from review

tests that even records with None at cursor path are loaded

bd46dcf

tests with full pipeline run instead of only extraction with table me…

a822499

…trics

Allows exclusion of None values for python Objects, not Pandas or Arrows

89118b0

refactors for speed and readability

d09436b

shortens implementation

e282c9f

suggestions from code review

30e3f1f

willi-mueller added 5 commits August 5, 2024 19:07

arrow: supports exclusion of rows where cursor value is missing

2820756

make test suite tighter

d127e46

arrow-table: processes null rows and adds them back after processing.…

6d7d4c0

… Fails for arrow-batch

supports include null cursor_path rows for RecordBatches

d4b8b68

ignores typing errors

ca7b91c

willi-mueller force-pushed the fix/1571-tolerate-record-without-incremental-cursor-path branch from 7feef42 to ca7b91c Compare August 7, 2024 14:06

willi-mueller requested a review from rudolfix August 7, 2024 14:07

fixes import error when arrow is not installed

d6fde19

willi-mueller self-assigned this Aug 8, 2024

willi-mueller added 2 commits August 8, 2024 22:37

code formatting

99c1e39

dissatisfying headstand to pass tests/pipeline/test_pipeline_extra.py…

06aff12

…::test_dump_trace_freeze_exception

willi-mueller force-pushed the fix/1571-tolerate-record-without-incremental-cursor-path branch from 2ddcd1b to 06aff12 Compare August 8, 2024 17:33

willi-mueller changed the title ~~Fix/1571 Incremental: Optionally load records with None value at cursor_path~~ Fix/1571 Incremental: Optionally load or ignore records with cursor_path missing or None value Aug 9, 2024

rudolfix requested changes Aug 13, 2024

View reviewed changes

dlt/extract/incremental/transform.py Outdated Show resolved Hide resolved

dlt/extract/incremental/transform.py Outdated Show resolved Hide resolved

dlt/extract/incremental/transform.py Outdated Show resolved Hide resolved

tests/extract/test_incremental.py Show resolved Hide resolved

willi-mueller added 3 commits August 20, 2024 16:11

extracts low-level code to method

a9d1775

simplifies code a bit

605461c

adds failing test with overlapping incremental cursor values.

0b267a2

willi-mueller force-pushed the fix/1571-tolerate-record-without-incremental-cursor-path branch from 589b592 to 0b267a2 Compare August 20, 2024 12:37

rudolfix added 3 commits August 21, 2024 13:50

drops temp arrow index before concat and any returns

9786abd

Merge branch 'devel' into fix/1571-tolerate-record-without-incrementa…

4a22f34

…l-cursor-path

handles non existing attr when gettin row_value, simplifies getter code

4ce384a

rudolfix approved these changes Aug 30, 2024

View reviewed changes

willi-mueller merged commit 1723faa into devel Aug 30, 2024
56 checks passed

willi-mueller deleted the fix/1571-tolerate-record-without-incremental-cursor-path branch August 30, 2024 17:31

willi-mueller restored the fix/1571-tolerate-record-without-incremental-cursor-path branch August 30, 2024 17:32

willi-mueller deleted the fix/1571-tolerate-record-without-incremental-cursor-path branch August 30, 2024 17:32

rudolfix mentioned this pull request Sep 13, 2024

1.0.0 announcement and release notes #1778

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/1571 Incremental: Optionally load or ignore records with cursor_path missing or None value #1576

Fix/1571 Incremental: Optionally load or ignore records with cursor_path missing or None value #1576

willi-mueller commented Jul 10, 2024 •

edited

Loading

netlify bot commented Jul 10, 2024 •

edited

Loading

VioletM commented Jul 29, 2024

rudolfix left a comment

willi-mueller commented Jul 30, 2024 •

edited

Loading

willi-mueller commented Aug 8, 2024 •

edited

Loading

rudolfix left a comment

rudolfix left a comment

Fix/1571 Incremental: Optionally load or ignore records with cursor_path missing or None value #1576

Fix/1571 Incremental: Optionally load or ignore records with cursor_path missing or None value #1576

Conversation

willi-mueller commented Jul 10, 2024 • edited Loading

Description

TODO

Related Issues

netlify bot commented Jul 10, 2024 • edited Loading

✅ Deploy Preview for dlt-hub-docs ready!

VioletM commented Jul 29, 2024

rudolfix left a comment

Choose a reason for hiding this comment

willi-mueller commented Jul 30, 2024 • edited Loading

willi-mueller commented Aug 8, 2024 • edited Loading

rudolfix left a comment

Choose a reason for hiding this comment

rudolfix left a comment

Choose a reason for hiding this comment

willi-mueller commented Jul 10, 2024 •

edited

Loading

netlify bot commented Jul 10, 2024 •

edited

Loading

willi-mueller commented Jul 30, 2024 •

edited

Loading

willi-mueller commented Aug 8, 2024 •

edited

Loading